Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed use of ECB with Botan #724

Closed
wants to merge 2 commits into from
Closed

Fixed use of ECB with Botan #724

wants to merge 2 commits into from

Conversation

antoinelochet
Copy link

Here is my proposed fix for issue 723

@antoinelochet antoinelochet changed the title #723 - Fixed use of ECB with Botan Fix issue 723 - Fixed use of ECB with Botan Sep 1, 2023
@antoinelochet antoinelochet mentioned this pull request Nov 7, 2023
Open
bjosv
bjosv reviewed Jan 16, 2024
View reviewed changes
Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.
Are all the extra debug printouts needed? I seems that existing printouts are quite sparse with information, maybe for security reasons(?).

As an addition I suggest that we also disable 2 testcases when built WITH_BOTAN.
They will always fail when using Botan as I understand it. WDYT?

1) test: AESTests::testECB (F) line: 518 AESTests.cpp
assertion failed
- Expression: aes->encryptInit(&aesKey128, SymMode::ECB, IV)


2) test: DESTests::testECB (F) line: 537 DESTests.cpp
assertion failed
- Expression: des->encryptInit(&desKey56, SymMode::ECB, IV)

src/lib/data_mgr/SecureDataManager.cpp Show resolved Hide resolved
@antoinelochet
Copy link
Author

antoinelochet commented Jan 16, 2024 •
edited
Loading

I think that you are right about the tests.

I have added the debug logs because they are often useful. I agree that some security reasons, it may seem bad, but:

  • using SoftHSM in production is madness
  • using SoftHSM in production WITH debug logs is beyond madness

So I thought this was tolerable. But per your request, I deleted all the offending lines.

bjosv
bjosv reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comments. Using debug logs in production would be "beyond madness" :)

src/lib/crypto/test/AESTests.cpp Outdated Show resolved Hide resolved
@bjosv bjosv mentioned this pull request Jan 18, 2024
Merged
@anpa8480
Copy link

anpa8480 commented Jun 21, 2024 •
edited
Loading

if you revert BotanSymmetricAlgorithm.cpp 8fd89ec and uncomment if (mode == SymMode::ECB) all tests are passing.

Tested with Botan 2.19.3
image

@antoinelochet
Copy link
Author

if you revert BotanSymmetricAlgorithm.cpp 8fd89ec and uncomment if (mode == SymMode::ECB) all tests are passing.

Tested with Botan 2.19.3 image

That's because ECB is not taken into account during compilation phase. It does not fix the failing tests in src/lib

@anpa8480
Copy link

anpa8480 commented Jun 21, 2024 •
edited
Loading

if you revert BotanSymmetricAlgorithm.cpp 8fd89ec and uncomment if (mode == SymMode::ECB) all tests are passing.
Tested with Botan 2.19.3 image

That's because ECB is not taken into account during compilation phase. It does not fix the failing tests in src/lib

Sorry I dont understand what do you mean "ECB is not taken into account". You mean because Botan_ecb.cpp is missing in CMakelists.txt? The test was performed with dev branch and not with fix-botan-ecb

@jschlyter jschlyter linked an issue Nov 29, 2024 that may be closed by this pull request
ECB is not supported by Botan #723
Open
@jschlyter jschlyter changed the title Fix issue 723 - Fixed use of ECB with Botan Fixed use of ECB with Botan Nov 29, 2024
@jschlyter jschlyter added the bug Some isn't right label Nov 29, 2024
@antoinelochet antoinelochet requested a review from a team as a code owner November 29, 2024 13:46
@antoinelochet antoinelochet marked this pull request as draft November 29, 2024 14:42
@antoinelochet
Copy link
Author

antoinelochet commented Nov 29, 2024 •
edited
Loading

I will close this PR when #771 is merged since it does a better job than this.

@antoinelochet
Copy link
Author

#771 is merged

@antoinelochet antoinelochet deleted the fix-botan-ecb branch November 29, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjosv bjosv bjosv left review comments

@zuiderkwast zuiderkwast zuiderkwast left review comments

Assignees
No one assigned
Labels
bug Some isn't right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECB is not supported by Botan
5 participants
@antoinelochet @anpa8480 @zuiderkwast @bjosv @jschlyter